Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Implement PHPStan #596

Closed
wants to merge 16 commits into from
Closed

[3.x] Implement PHPStan #596

wants to merge 16 commits into from

Conversation

Jade-GG
Copy link
Collaborator

@Jade-GG Jade-GG commented Oct 16, 2024

Adds PHPStan as a workflow and fixes all (current) errors that come out of PHPStan level 8.

Includes some other fixes that I happened to come across, for example dynamic models not being used properly.

I've done a quick test on an existing project to see if I've made any errors, but I could not spot any. Nevertheless, this is of course still a hugely breaking change.

.github/workflows/analyse.yml Outdated Show resolved Hide resolved
src/Auth/MagentoCustomerTokenGuard.php Show resolved Hide resolved
src/Commands/IndexCategoriesCommand.php Outdated Show resolved Hide resolved
src/Events/IndexAfterEvent.php Show resolved Hide resolved
src/Http/Controllers/Fallback/UrlRewriteController.php Outdated Show resolved Hide resolved
src/Listeners/Healthcheck/ModelsHealthcheck.php Outdated Show resolved Hide resolved
src/Models/Config.php Show resolved Hide resolved
src/Models/Scopes/IsActiveScope.php Outdated Show resolved Hide resolved
@@ -261,7 +274,7 @@ protected function bootListeners(): self
protected function bootMacros(): self
{
Collection::macro('firstForCurrentStore', function () {
/** @var Collection $this */
/** @var Collection<> $this */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works out to a Collection<mixed> type by default, which is what you want here with the macro. I could specify it explicitly if preferable?

phpstan.neon Show resolved Hide resolved
src/Actions/DecodeJwt.php Outdated Show resolved Hide resolved
src/Http/Controllers/FallbackController.php Outdated Show resolved Hide resolved
@royduin
Copy link
Member

royduin commented Oct 17, 2024

Do we really want level 8 with all those ignores? And a bit to many docblocks if you ask me. Laravel itself is running level 1: https://github.com/laravel/framework/blob/11.x/phpstan.src.neon.dist

@BobWez98
Copy link
Collaborator

I agree with @royduin . Also quite a few ignores. Rather have a lower level then this much ignores.

@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Oct 21, 2024

I've lowered it to level 1, allowing me to also make a specific ignore for all eventy errors, which removed all but one of the remaining phpstan ignores 🙂

@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Oct 29, 2024

#620

@Jade-GG Jade-GG closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants